Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[INTEL MKL] Fixed missing copts in BUILD file. #32317

Merged
merged 1 commit into from Sep 10, 2019
Merged

[INTEL MKL] Fixed missing copts in BUILD file. #32317

merged 1 commit into from Sep 10, 2019

Conversation

agramesh1
Copy link
Contributor

The -DINTEL_MKL compiler option was not getting passed to mkl related eager files (removed in a prior commit.) Added tf_copts to the build, tf_copts includes -DINTEL_MKL.

@tensorflow-bot tensorflow-bot bot added the size:XS CL Change Size: Extra Small label Sep 7, 2019
@agramesh1
Copy link
Contributor Author

agramesh1 commented Sep 7, 2019 •
edited

Pinging @mahmoud-abuzaina and @penpornk

@penpornk penpornk assigned penpornk and unassigned penpornk Sep 7, 2019
@penpornk penpornk self-requested a review September 7, 2019 16:48
penpornk
penpornk reviewed Sep 7, 2019
View reviewed changes
Copy link
Member

@penpornk penpornk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good to me. I'm looping in @gunan and @scentini for "-fno-exceptions" in #31599.

Background:
This change is to be cherry-picked into r2.0 if possible, to re-enable MKL in eager execution (I accidentally disabled it when fixing some build failures -- my bad!). ddde447 and e5f8043 are not in r2.0. Is it okay to use "-fno-exceptions" here for now?

@gbaned gbaned self-assigned this Sep 9, 2019
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Sep 9, 2019
@agramesh1 agramesh1 mentioned this pull request Sep 9, 2019
Closed
@scentini
Copy link
Contributor

scentini commented Sep 9, 2019

The use of nocopts will break the TF build with Bazel 1.0. As 1.0 is not yet released, this PR will not cause immediate issues. However, I suggest to have a follow-up PR that changes tf_copts() to tf_copts(allow_exceptions = True) and removes the nocopts attribute ASAP, so you can both cherrypick this PR and we are still on the right track for 1.0 migration.

@mahmoud-abuzaina
Copy link
Contributor

Thanks @scentini for the explanation. Yes, I am going to submit another PR soon that will use tf_copts(allow_exceptions = True) to be merged with master. But since the r2.0 still does not support that argument, we need this PR to be cherry-picked into r2.0.

@penpornk
Copy link
Member

penpornk commented Sep 9, 2019

@scentini Thank you for taking a look!
@mahmoud-abuzaina To save time, I can submit the change internally after this PR is merged into the master branch. :)
Approving this PR now.

PR Queue automation moved this from Assigned Reviewer to Approved by Reviewer Sep 9, 2019
@tensorflow-bot tensorflow-bot bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Sep 9, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Sep 9, 2019
@mahmoud-abuzaina
Copy link
Contributor

@penpornk thank you so much. That will be great.

@penpornk penpornk added the kokoro:force-run Tests on submitted change label Sep 10, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Sep 10, 2019
tensorflow-copybara pushed a commit that referenced this pull request Sep 10, 2019
@tensorflower-gardener
Merge pull request #32317 from Intel-tensorflow:agramesh/mkl_eager_fix
7d2873b 
PiperOrigin-RevId: 268172536
@tensorflow-copybara tensorflow-copybara merged commit c6d1935 into tensorflow:master Sep 10, 2019
PR Queue automation moved this from Approved by Reviewer to Merged Sep 10, 2019
tensorflow-copybara pushed a commit that referenced this pull request Sep 10, 2019
@penpornk @tensorflower-gardener
Make mkl_eager_op_rewrite build rule compatible with Bazel 1.0, per t…
a25803e 
…he discussion in PR #32317:

#32317#issuecomment-529667983

PiperOrigin-RevId: 268235250
This was referenced Sep 11, 2019
Merged
Merged
@nammbash nammbash deleted the agramesh/mkl_eager_fix branch June 16, 2020 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@penpornk penpornk penpornk approved these changes

@alextp alextp Awaiting requested review from alextp

@jaingaurav jaingaurav Awaiting requested review from jaingaurav

Assignees

@gbaned gbaned

Labels
cla: yes ready to pull PR ready for merge process size:XS CL Change Size: Extra Small
Projects
PR Queue
  
Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@agramesh1 @scentini @mahmoud-abuzaina @penpornk @googlebot @kokoro-team @tensorflow-copybara @gbaned